feat(routing): upgrade to GraphHopper 8.0 #363

kschrab · 2023-11-02T15:03:32Z

Description

This PR upgrades the GraphHopper library to version from 0.13.0 (2019) to latest release 8.0.
Some easy migration paths involved renamings (e.g., QueryResult -> Snap)
The FlagEncoder has been removed completely, and was replaced by a collection of EncodedValue classes. The FlagEncoder previously just read one or more integer flags from an edge and encoded/decoded information, such as allowed speed or the waytype. Now, we must use an EncodedValue implementation for every aspect. For example, VehcileSpeed or VehicleAccess, which exist for encoding the average speed and the access to use a certain edge. For each vehicle type (here called profile, e.g. "car", or "bike"), individual instances of these EncodedValue types exist. This lead to a rather large refactoring, but could be solved by adding a new VehicleEncoding class which just bundles these individual EncodedValue types needed for a profile/vehicle.
Previously, the route functionality was implemented twice. First, as a direct extension of the GraphHopper implementation (which serves as an API end point), and secondly, in GraphHopperRouting which implements the the mosaic-routing API. I removed the implementation in ExtendedGraphHopper and kept only the one in GraphHopperRouting. Therefore ExtendedGraphHopper currently only replaces the import mechanism of the GraphHopper implementation to create the routing graph from a mosaic database. I removed the implementation ExtendedGraphHopper completely and moved everything related to graph loading to GraphHopperRouting, having one entry point.
All additional routing algorithm implementations based on AbstractCamvitChoiceRouting have been removed. With the current release, GraphHopper already comes with plateau based algorithms to search for alternative routes. These are more or less based on the Camvit Choice Routing algorithm and therefore expect to produce similar results. To avoid a complicated migration of our own implementations, we use the provided ones for now.
After migration, some tests related to turn costs now result in slightly different costs. I could not yet find a reason for this, but I still feel that the guessed turn costs are okay.

Issue(s) related to this PR

Resolves internal issue 716

Affected parts of the online documentation

Changes in the documentation required?

No

Definition of Done

Prerequisites

You have read CONTRIBUTING.md carefully.
You have signed the Contributor License Agreement.
Your GitHub user id is linked with your Eclipse Account.

Required

The title of this merge request follows the scheme type(scope): description (in the style of Conventional Commits)
You have assigned a suitable label to this pull request (e.g., enhancement, or bugfix)
origin/main has been merged into your Fork.
Coding guidelines have been followed (see CONTRIBUTING.md).
All checks on GitHub pass.
All tests on Jenkins pass.

Requested (can be enforced by maintainers)

New functionality is covered by unit tests or integration tests. Code coverage must not decrease.
If a bug has been fixed, a new unit test has been written (beforehand) to prove misbehavior
There are no new SpotBugs warnings.

Special notes to reviewer

Signed-off-by: Karl Schrab <[email protected]>

…ightingFactory Signed-off-by: Karl Schrab <[email protected]>

Signed-off-by: Karl Schrab <[email protected]>

pom.xml

…HopperRouting instead Signed-off-by: Karl Schrab <[email protected]>

Signed-off-by: Karl Schrab <[email protected]>

… utils Signed-off-by: Karl Schrab <[email protected]>

…afficgen Signed-off-by: Karl Schrab <[email protected]>

# Conflicts: # lib/mosaic-routing/src/main/java/org/eclipse/mosaic/lib/routing/graphhopper/GraphHopperRouting.java

cutting decimal places

…old behaviour

schwepmo

Some remarks.

NOTICE-THIRD-PARTY.md

lib/mosaic-routing/src/main/java/org/eclipse/mosaic/lib/routing/CandidateRoute.java

lib/mosaic-routing/src/test/java/org/eclipse/mosaic/lib/routing/CharlottenburgRoutingTest.java

lib/mosaic-routing/src/main/java/org/eclipse/mosaic/lib/routing/database/DatabaseRouting.java

lib/mosaic-routing/src/main/java/org/eclipse/mosaic/lib/routing/CandidateRoute.java

...c-routing/src/main/java/org/eclipse/mosaic/lib/routing/graphhopper/util/VehicleEncoding.java

...routing/src/main/java/org/eclipse/mosaic/lib/routing/graphhopper/util/TurnCostsProvider.java

...g/src/test/java/org/eclipse/mosaic/lib/routing/graphhopper/util/DatabaseGraphLoaderTest.java

...-routing/src/test/java/org/eclipse/mosaic/lib/routing/graphhopper/GraphHopperMosaicTest.java

.../test/java/org/eclipse/mosaic/lib/routing/graphhopper/algorithm/CamvitChoiceRoutingTest.java

…fter review

schwepmo · 2024-03-21T12:38:05Z

lib/mosaic-routing/src/main/java/org/eclipse/mosaic/lib/routing/CandidateRoute.java

    }

+    /**
+     * @return the list of connection ids forming this route.


I think these type of javadocs leads to spotbugs warnings, as the actual documentation is missing.
Personally, I think the documentation for getters should either be really verbose (like the member variable) or none.

In this particular case I'd need the documentation at the getters, since the user wants to know what the offset is doing. However, for getters I find it personally more useful to fill @return and leave the description blank, as this would be the same text. I'd rather adjust our checkstyle...

schwepmo · 2024-03-21T12:40:30Z

...uting/src/test/java/org/eclipse/mosaic/lib/routing/graphhopper/GraphHopperWeightingTest.java

@@ -0,0 +1,159 @@
+/*
+ * Copyright (c) 2020 Fraunhofer FOKUS and others. All rights reserved.


Do we have to adjust this year, as this file is marked as "new"?

No we don't have to do this.

schwepmo · 2024-03-21T12:41:16Z

.../java/org/eclipse/mosaic/lib/routing/graphhopper/algorithm/AlternativeRoutesRoutingTest.java

@@ -0,0 +1,128 @@
+/*
+ * Copyright (c) 2020 Fraunhofer FOKUS and others. All rights reserved.


Do we have to adjust this year, as this file is marked as "new"?

kschrab added 7 commits October 31, 2023 14:41

feat(routing): upgrade GraphHopper to 8.0, several refactorings

354edb0

Signed-off-by: Karl Schrab <[email protected]>

fix(routing): fixed speed conversion

e29df56

Signed-off-by: Karl Schrab <[email protected]>

fix(routing): allow configuring alternative route parameters

2ca4bee

Signed-off-by: Karl Schrab <[email protected]>

test: reverted accidental commit

b587e47

Signed-off-by: Karl Schrab <[email protected]>

feat(routing): reactivate cleanup, move weighting instantiation to we…

1d19f29

…ightingFactory Signed-off-by: Karl Schrab <[email protected]>

fix(routing): resolved compilation error

1a16932

Signed-off-by: Karl Schrab <[email protected]>

fix(routing): removed double configuration of alternative route params

1b00f5d

Signed-off-by: Karl Schrab <[email protected]>

kschrab added the chore Pull request that resolve around a chore label Nov 2, 2023

fix(routing): resolve spotbugs warnings

ce8cc96

Signed-off-by: Karl Schrab <[email protected]>

kschrab marked this pull request as ready for review November 2, 2023 15:17

kschrab requested review from schwepmo and rprotzmann November 2, 2023 15:17

kschrab commented Nov 2, 2023

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

kschrab added 11 commits November 3, 2023 09:18

refactor(routing): get rid of ExtendedGraphHopper, load grap in Graph…

f242643

…HopperRouting instead Signed-off-by: Karl Schrab <[email protected]>

legal: approve third party content

d09edd4

Signed-off-by: Karl Schrab <[email protected]>

clean: re-added valuable comment about virtual edges

f519de7

Signed-off-by: Karl Schrab <[email protected]>

clean: removed unused GraphLoader interface, moved VehicleEncoding to…

fc608a0

… utils Signed-off-by: Karl Schrab <[email protected]>

clean: make alternative routing parameters accessible to alter via tr…

5714d91

…afficgen Signed-off-by: Karl Schrab <[email protected]>

Merge remote-tracking branch 'origin/main' into 716-graphhopper-8

2a29681

# Conflicts: # lib/mosaic-routing/src/main/java/org/eclipse/mosaic/lib/routing/graphhopper/GraphHopperRouting.java

fix(routing): revive changes overridden by merge

4b297ea

fix(mosaic-routing): calcEdgeWeight should return values as seconds

ed25671

fix(mosaic-routing): round max speed before

e8223d5

cutting decimal places

test(mosaic-routing): use 60 km/h as speed for test graph to restore …

c3819c0

…old behaviour

clean(mosaic-routing): minor cleanup after self-review

5a685f4

schwepmo reviewed Mar 20, 2024

View reviewed changes

kschrab self-assigned this Mar 20, 2024

kschrab added 3 commits March 20, 2024 17:40

Merge remote-tracking branch 'origin/main' into 716-graphhopper-8

1fcfaf0

fix(routing): fixed waytype encoding and addressed further cleanups a…

7128d75

…fter review

fix(routing): suppress spotbugs warning

dc8deb0

schwepmo reviewed Mar 21, 2024

View reviewed changes

schwepmo approved these changes Mar 21, 2024

View reviewed changes

schwepmo merged commit 7bdf6ae into main Mar 21, 2024
6 checks passed

schwepmo deleted the 716-graphhopper-8 branch March 21, 2024 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(routing): upgrade to GraphHopper 8.0 #363

feat(routing): upgrade to GraphHopper 8.0 #363

kschrab commented Nov 2, 2023 •

edited

Loading

schwepmo left a comment

schwepmo Mar 21, 2024

kschrab Mar 21, 2024 •

edited

Loading

schwepmo Mar 21, 2024

kschrab Mar 21, 2024

schwepmo Mar 21, 2024

		@@ -0,0 +1,159 @@
		/*
		* Copyright (c) 2020 Fraunhofer FOKUS and others. All rights reserved.

		@@ -0,0 +1,128 @@
		/*
		* Copyright (c) 2020 Fraunhofer FOKUS and others. All rights reserved.

feat(routing): upgrade to GraphHopper 8.0 #363

feat(routing): upgrade to GraphHopper 8.0 #363

Conversation

kschrab commented Nov 2, 2023 • edited Loading

Description

Issue(s) related to this PR

Affected parts of the online documentation

Definition of Done

Special notes to reviewer

schwepmo left a comment

Choose a reason for hiding this comment

schwepmo Mar 21, 2024

Choose a reason for hiding this comment

kschrab Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

schwepmo Mar 21, 2024

Choose a reason for hiding this comment

kschrab Mar 21, 2024

Choose a reason for hiding this comment

schwepmo Mar 21, 2024

Choose a reason for hiding this comment

kschrab commented Nov 2, 2023 •

edited

Loading

kschrab Mar 21, 2024 •

edited

Loading